-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reorganize In-App Dashboard & Use New Energy Footprint Calculations #1176
Conversation
Move the components used by a 'row' of cards to a subdirectory of 'metrics' Create new components for each section which groups the cards into a Carousel
Renamed "Summary" section to "Travel" section; this will show 'distance', 'duration', and 'count' Update types to include 'footprint' as a metric, and revise the sections ('footprint', 'movement', 'surveys', 'travel')
- update color palette to align with Material Design 3 (which is what React Native Paper is based on) - update NavBar component; pass props through to the underlying Appbar.Header, allowing flexibility of elevated = true or false
includes a fix for 'validateDOMNesting' error message
Moved to a more general location since this can be used in many parts of the UI
height = 40 & adjust padding/margins to match standard Material UI button use lighter grey on DateSelect, show on one line and use month + day (e.g. "Sep 9") instead of MM/DD/YYYY
Speeds up development; does not affect production
We had used <Modal> here (while still in the process of migrating from Angular) to ensure the snackbar shows above the rest of the content. This prevented interacting with other content until the snackbar (and its modal) were dismissed. Per RN Paper docs, Snackbars are intended to work with Portals. https://callstack.github.io/react-native-paper/docs/components/Snackbar/ We can do this now that we have a PaperProvider at the App level (index.js)
Bars on a chart get shown with transparency if they begin with this. In a previous version, it was dashed lines, but it's no longer apt to call it dash_key.
This mostly just matters for: 1) testing with historical data, or 2) people whose pipeline is stuck, which indicates a deeper problem. But it will prevent any scenarios where several weeks/months get loaded in all at once. To aid with scenario (1), we can add a button to the TimelineScrollList: if there is no travel in the past week and the pipeline end is not in the last week, this gives us a shortcut to the last processed week
both Label and Dashboard tabs use date & time formatting functions using ISO strings, including with/without weekday, and with/without year, and "humanized" duration strings Makes generic versions of these and puts them in js/util.ts DateSelect will use this as well; as a result it will shift from using a numerical, with year (e.g. "9/17/2024") representation, to an abbreviated string form (e.g. "Sep 17") This is 1) more friendly / less initimidating, 2) allows the datepicker text to be one line, which reduces clutter
If there is an long stretch of time with no data, this implementation will not work; it should keep inserting empty weeks until there are no more days
Categorizing this as "Movement" instead of "Active Travel" Also updating to determine the list of "active modes" from the presence of "met" in the rich modes. Before, this was a config option or defaulted to only "walk" and "bike" (now it will include "ebike" if "ebike" has non-zero "mets", or any other modes with non-zero "mets") -update styles on the cards
When the user first goes to the Dashboard in an app session, we want to show them at least 2 weeks so they can compare the previous week to the past week. But after that we should allow them to select any range. So we can add an 'isInitialized' bool state to mark this, and only do this ">= 14 days" check on the initialization Also add a 'refresh' function which not only refreshes the timeline but also resets 'isInitialized' (along with 'aggMetricsIsLoading')
and fix them since they were flipped ('low' should come first)
e-mission-common 0.6.1 comes with expanded metrics (support for 'footprint', which needs label_options passed from MetricsTab.tsx). Update types: new LabelOptions spec, explicit type for RichMode, add 'footprint' to metricsTypes, add "MetricEntry" type which is like DayOfMetricData but not necessarily for 1 day (it could be aggregated across an entire week, for example) Add "goals" to appconfig.metrics.phone_dashboard_ui.footprint_options
Also fixes translation issue: instead of checking for hardcoded value "Unlabeled" (which probably did not work in other languages) check if the label starts with whatever the "unlabeled" string is for the current language
Shows barchart of average daily footprint for the user, grouped by weeks, colored based on goals ("meter" of green -> red) Includes checkboxes to break down by a grouping field (mode_confirm, purpose_confirm, etc) If mode_confirm, then base mode colors are used instead of the 'meter' Works for either 'carbon' (with kg_co2) or 'energy' (with kwh) new functions in metricsHelper - aggMetricEntries combines/ merges array of MetricEntry (used by WeeklyFootprintCard to aggregate multiple days into a week) - sumMetricEntry adds up all values within a day - sumMetricEntries does both The type definitions may look complex here, but all they really do is allow the return type to depend on the 'metricName'. For example, if metricName is 'duration', then the return type is just a number. But if metricName is 'footprint', it is an object containing 'kwh' and 'kg_co2' This way we don't have to write separate functions for every metric
use the new components, SummaryCard and WeeklyFootprintCard, on the Footprint section. Each of these work for both carbon and energy, so we don't need the 'energy' components, nor the old CarbonFootprintCard and CarbonTextCard A few things of note in FootprintSection: cumulativeFootprintSum sums the footprint metric (which includes both carbon and energy) across all days. This gets fed into SummaryCard Goals are read from the appconfig. Because they are internationalized, though, the labels have to looked up by lang. If a footnote is configured, it is added and a reference to it is appended to the label. The goals are passed to SummaryCard and WeeklyFootprintCard A dynamic footnotes mechanism via `addFootnote`. Instead of including the footnote numbers, (e.g. ¹ ²) directly in the strings, we can round them up for the whole section, keeping track of their numbers, and show them all at the bottom. This allows footnotes to be dynamically shown. If one configuration doesn't use "goals", then that footnote won't show. The "unlabeled" footnote will then become ¹, avoiding any weird scenarios where ¹ is not present and it skips straight to ² We can expand on this later if we want to be more detailed about footnotes, references, data sources
used by WeeklyFootprintCard
instead of setting dateRange only once pipelineRange is set, we can set dateRange upfront and just not do anything with it until pipelineRange has been set This saves us from constantly checking dateRange != null anywhere it is used downstream
It needs to be MultilabelKey by default, otherwise we have to specify this everywhere
Unifies the styling of all metrics cards Adjusts navbar and refresh button on label screen so the label screen and metrics screen navbars match each other
Where base modes were previously a property of the label options, and had a separate set of properties, we now have rich modes that inherit from base modes and may include other properties. This means for the numerous places in the UI where we use mode colors, we should always check the rich mode via get_rich_mode / get_rich_mode_for_value. Also we can tighten down typings a bit more. "text" does not exist in label options or in rich modes. It had been added in while label options are being read and the translations are parsed Instead of that let's just have a function that does this (labelKeyToText). We need to check for translations in 1) the label options being used; and 2) the default label options. if nothing, then convert it to readable (convert underscores to spaces & capitalize) We'll also have a function that does the opposite. Updated some typings: LabelOptions type should not accept a parameter. Also should mark REPLACED_MODE as optional UserInputData should not have "name". This only exists on EnketoUserInputData; it represents the name of the survey
not just the last 14 days
give default argument to MetricEntry and DayOfMetricData so we don't have to specify every time Only ['count', 'distance', 'duration'] are used on the TravelSection; add type TravelMetricName to reflect this I have not added unit utils for 'footprint' yet; but currently it is only used for ['count', 'distance', 'duration'] anyway
There are some places where we want an instance of colors accessible from outside a component, ie. not via useAppTheme
Makes this implementation match what the existing tests are expecting Updates comment since it was out of date
gives a "You vs. Group" comparison for average daily carbon and energy
we do not need to lookup the label by language beacuse getFootprintGoals already handles this
This is necessary to be able to appropriately divide aggregate metrics back to per-user scale for "You vs. Group" comparison
customFootprint not needed anymore; replaced by emcommon footprint calculations aka CHEER 'mets' are not currently in use, but the data in 'metDataset.ts' is included in the base modes in emcommon, so we do have this available to add back in the future
confirmHelper 'json/label-options.json.sample' does not exist anymore so let's just test the default, if we passed a blank config diaryHelper Most of the functions that were tested here don't exist anymore and the tests can be removed. In fact, getDetectedModes is the only one left. However there are functions that didn't have tests, add those footprintHelper All of these functions were removed and there is only one new one, getFootprintGoals metricsHelper The date format changed from numerical month/day to abbreviated month/day imperialConfig Extract the imperialConfig generation to a separate function so it can be accessed outside the hook. Simpler and makes it easier to test. Also let's test both metric and imperial, not just metric
…into energy-dashboard-2024
if acc is not defined or is a number (as with distance,duration,count), do not add nUsers
We created this test a while ago intended as an example. We now have other component tests and don't need an example lying around
I added the StatusBar plugin so we can change the color to match the appbar color
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1176 +/- ##
==========================================
- Coverage 30.14% 28.83% -1.32%
==========================================
Files 118 122 +4
Lines 5172 5012 -160
Branches 1110 1145 +35
==========================================
- Hits 1559 1445 -114
+ Misses 3611 3565 -46
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Although the patch coverage is higher than the project coverage, the overall project coverage drops with this PR. I added unit tests for new functions, with |
Adds translations needed for e-mission/e-mission-phone#1176, plus other translations that I have noticed are missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So glad that I just have a do a cursory review. At this point, my plan is to just merge, but I will flag any high-level issues to be addressed in a future cleanup.
The screenshots/design look great; do you have an example of what the survey screen looks like?
"e-mission-common": "github:JGreenlee/e-mission-common#semver:0.5.4", | ||
"e-mission-common": "github:JGreenlee/e-mission-common#semver:0.6.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should really review the e-mission-common changes at some point too. Maybe I can do that while generating the results for the paper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the easiest way to do that would be looking at the releases on e-mission-common and the "Full changelog"s that are generated there
https://github.com/JGreenlee/e-mission-common/releases
@@ -63,6 +63,13 @@ const EnketoModal = ({ surveyName, onResponseSaved, opts, ...rest }: Props) => { | |||
useEffect(() => { | |||
if (!rest.visible || !appConfig) return; | |||
initSurvey(); | |||
|
|||
// on dev builds, allow skipping survey with ESC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so cool! ⏩ This will affect the demographics survey for sure, but won't it also affect the other trip-level surveys?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will affect them. This change was just focused on making testing in emulator easier, which was only cumbersome with the onboarding survey.
The trip-level surveys have a "dismiss" button, while the first time through the onboarding survey does not (by design)
I don't know that the check for __DEV__
was really needed, given that it is pretty hard to press ESC
on a mobile app. But I did the principled thing and made sure it is only possible on dev builds
* @example getFormattedDate("2023-07-14T00:00:00-07:00") => "Jul 14, 2023" | ||
* @example getFormattedDate("2023-07-14", "2023-07-15") => "Jul 14, 2023 - Jul 15, 2023" | ||
*/ | ||
export function formatIso(isoStr1?: string, isoStr2?: string, opts?: Intl.DateTimeFormatOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't there standard "humanize" functions for this? Are they not good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using Luxon's toLocaleString
(which is like a wrapper around the standard Intl
API's toLocaleString
), which is doing most of the work here
The functions here exist for 2 reasons:
i) To format date ranges (e.g. Sep 17, 2024 – Sep 23, 2024
) instead of only a single date (e.g. Sep 23, 2024
)
ii) Shortcuts so I don't have to pass something like { month: 'short', day: 'numeric', year: 'numeric' }
every time
color: baseMode.color, | ||
icon: baseMode.icon, | ||
color: (confirmedModeForTrip || base_modes.BASE_MODES['UNPROCESSED']).color, | ||
icon: (confirmedModeForTrip || base_modes.BASE_MODES['UNPROCESSED']).icon, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Were we indeed only showing the basemode color before? Also, if this is actually the confirmed color instead of the confirmed mode object, you may want to use confirmedColorForTrip
for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color
was solely a property of the base mode before. Rich modes could not have their own color separate from the base mode.
So previously it was using the color of the basemode of the confirmed mode. Now it is the color or the rich confirmed mode.
* @param metricName the metric that the values are for | ||
* @returns the result of summing the values across all fields in the entry | ||
*/ | ||
export function sumMetricEntry<T extends MetricName>(entry: MetricEntry<T>, metricName: T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this use aggMetricEntries
? I guess because we don't need the aggregation and it is extra work to aggregate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aggMetricEntries: aggregates/sums across entries
sumMetricEntry: sums within 1 entry
sumMetricEntries: does both
Highlights / High level summary of changes
New dashboard UI, which uses the the footprint calculations from e-mission-common (aka CHEER)
Footprint section
Weekly emissions & energy use can be broken down by
mode
(theoreticallypurpose
as well, if configured)Movement section
Shows however many weeks are loaded, rather than just the past 2 weeks (although the default initial load is still only 2 weeks)
Travel section
No major changes besides UI
Miscellaneous
Timeline always initializes to the last 7 days. There is a shortcut to jump to the last processed week if the timeline is empty (useful for testing historical data)
SnackBars no longer block interaction with the UI while visible
AppBar follows Material Design 3 guidelines. StatusBar matches as well. Date display is more user-friendly and less cluttered